Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

628: Support XLIFF tags in android imports. #636

Merged
merged 17 commits into from
Mar 21, 2017

Conversation

toolstack
Copy link
Contributor

Resolves #628.

@toolstack toolstack added this to the 2.3 milestone Jan 14, 2017
Copy link
Member

@ocean90 ocean90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a unit test with an example XML file.

*
* See the "Mark message parts that should not be translated" section of https://developer.android.com/distribute/tools/localization-checklist.html
*
* Unfortunatly simplexml will parse these are valid XML tags, which we don't want so encapsalate them in a CDATA structure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typos:

  • Unfortunatly => Unfortunately
  • encapsalate => encapsulate
  • simplexml => SimpleXML

@toolstack
Copy link
Contributor Author

Unit test added.

Copy link
Member

@ocean90 ocean90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The export doesn't look right:

<resources>
	<string name="with_xliff">Please don\'t translate &lt;xliff:g id="excluded" example="this">this&lt;/xliff:g> text</string>
</resources>

@toolstack
Copy link
Contributor Author

That's because the current code encodes all less than signs as HTML entities (see https://github.com/GlotPress/GlotPress-WP/blob/develop/gp-includes/formats/format-android.php#L164). It will get "worse" once we include double quotes in the escaping as well.

The current code assumes that only displayable text is in the export.

I would think that with the current GP workflow that the originals import would contain the xliff tag and then the translators would remove it from the translation. That would maintain the current assumption about the output.

We could do a similar hack and after we've encoded the less than signs, decode any "<xliff:g"'s that exist, but that will become much more cumbersome with the double quote escaping happening.

In an ideal world, when we imported the originals we'd parse the xliff tag and store the metadata somewhere else so the editor could "enforce" the non-editable text in the translation.

@ocean90 ocean90 modified the milestones: 2.3, Future Jan 17, 2017
@toolstack toolstack force-pushed the 628-support-xliff-in-android-files branch from 757965c to 2912410 Compare February 10, 2017 21:55
@toolstack
Copy link
Contributor Author

Ok, I've updated the PR to change the behaviour of the xliff tags.

The previous code simply kept them as part of the imported text, the new code instead strips them out and adds a comment to indicate what should be done.

For example, if you import an original with the following string in the xml file:

<string name="FileExistsMsg">A file with the specified name (<xliff:g id="filename" example="spreadsheet.xls">%s</xliff:g>) already exists.</string>

the importer will create this original:

A file with the specified name (%s) already exists.

but also add this comment to the translation:

This string has content that should not be translated, the "%s" component of the original, which is identified as the "filename" attribute by the developer may be replaced at run time with text like this: spreadsheet.xls

The translation row the looks like:
xliff-example

I haven't tracked down why the comment doesn't wrap but instead forces the entire meta area to appear below the original/translation, but that should be just a css rule somewhere (any suggestions on that one would be appreciated).

@toolstack
Copy link
Contributor Author

@ocean90 the new code should export cleanly now that the xliff tags have been removed from the strings.

@pdalfarr
Copy link

Great. When will this be available as 'worpress plugin update'?
Thanks !

@toolstack
Copy link
Contributor Author

@pdalfarr that depends, assuming it gets merged, we currently don't have a date for the next release.

If you want to give it a try though there's only one file you need to replace in your local install.

@pdalfarr
Copy link

@toolstack Import is now OK, thanks!
But export only allow to retrieve '%s'. xliff node seems to be lost :/

@toolstack
Copy link
Contributor Author

@pdalfarr Correct, since the xliff is only there to let the translators know what to do with the component, there's no need to export it in the final translation file.

@ocean90
Copy link
Member

ocean90 commented Feb 13, 2017

I'm pretty sure that the tag has to stay. The docs mention another example:

<string name="promo_message">
    Please use the "<xliff:g id="promotion_code">ABCDEFG</xliff:g>” to get a discount.
</string>

@pdalfarr
Copy link

pdalfarr commented Feb 13, 2017

@ocean90 yes, I think tags has to remains.
Other tools allow to export xliff .. (example: https://support.crowdin.com/api/export-file/ )

But, most important, android translated strings.xml usually contain xliff node if source string.xml does.
This is how it's done is most apps. And this is how it's done in the Android SDK itself.

Example online (from Android platform framework base source):

./values/strings.xml contains:

<-- Format string used to add a suffix like "KB" or "MB" to a number
    to display a size in kilobytes, megabytes, or other size units.
    Some languages (like French) will want to add a space between
    the placeholders. -->
<string name="fileSizeSuffix"><xliff:g id="number" example="123">%1$s</xliff:g><xliff:g id="unit" example="KB">%2$s</xliff:g></string>

and
./values-fr/strings.xml contains:

<string name="fileSizeSuffix" msgid="9164292791500531949">"<xliff:g id="NUMBER">%1$s</xliff:g> <xliff:g id="UNIT">%2$s</xliff:g>"</string>

This means that translations can contains xliff:g with some differences.

In this example, the '%2$s' is present in all translations, but it can be 'wrapped' into xliff nodes with different id value, depending on translation.
As you can see it in this example, some translations can contains 'example' xliff attribute, while other do not contains 'example' attribute.

So I think xliff nodes should be kept when exporting any files, may it be the reference language, but also per language.

This is my understanding of the Android xliff usage. @toolstack What is your opinion about that?

Thanks a lot!

Pascal

@toolstack
Copy link
Contributor Author

toolstack commented Feb 13, 2017

@ocean90 that example is not really any different than any of the others, just what text is not to be translated.

@pdalfarr No doubt you can export the xliff's again, but they're never used by an Android app from my understanding, they're there only to let translators know what not to translate. They would never change based on which language you were translating to.

If you're going to use a centralized tool like GP to do the translation, then you would import the xliff's as part of the originals, then only do the exports when you're ready to release the app. You wouldn't let your contributors directly edit the xml files but instead direct them to contribute to your GP install.

This is caused by a fundamental difference between the workflows for po/mo files (which contain both the original and the translation) and Android XML files, which only contain the translation.

As GP is designed around several assumptions based on the gettext standard, there's a bit of shoehorning to get Android or other formats that don't share the same assumptions to work in GP.

Storing the xliffs and re-exporting them would require significant changes to GP.

@pdalfarr
Copy link

@ocean90 Thanks for the reply.

I understand your point of view, and "Storing the xliffs and re-exporting them would require significant changes to GP" close the discussion then ;-)

The new functionality to take xliff into account is really great. The additional one I just mentioned was just 'cherry on the cake'. Without it, the cake is still really tasty :-)

Thanks

PS: I tested your WordPress GP Require Login plugin: it's working fine. Thanks!

@toolstack
Copy link
Contributor Author

@pdalfarr We've had discussions on "non-translatable" areas before and it is on the radar, just not a priority yet.

Glad the plugin did what you needed.

@toolstack toolstack added this to the 2.4 milestone Feb 14, 2017
@toolstack toolstack removed this from the Future milestone Feb 14, 2017
@toolstack
Copy link
Contributor Author

toolstack commented Feb 15, 2017

I've updated the PR to fix the css issue as well as add some additional test cases and support single quote attributes in the xliff tag.

@ocean90 when you get some time take a look at the css "fix" it changes the behaviour of the translation row a bit but I think it works better overall.


.editor .strings .textareas textarea {
width: 95%;
resize: none;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be resize: vertical to allow users to resize the textarea vertically if a string has more than ~7 lines.

* @param obj $string The string entry object to use.
* @param string $context The context string to use.
*
* @return obj A translation entry object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obj should be Translation_Entry.

*
* @since 1.0.0
*
* @param obj $string The string entry object to use.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like $string is an array.

Generally, if the type is an object than object should be used.

*
* @param obj $locale The locale object to use for exporting.
*/
private function get_plural_order( $locale ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be unit tested. I was under the impression that this info cannot be generated but needs to be read from a source like CLDR. In my tests with Russian it returned array( 'other', 'other', 'other' ) for 1,21,31, 2,3,4, and 0,5,6. Would be great to see this working without an external dependency, as I wanted to implement stringsdict for iOS which needs the same classification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the code in that function is primarily a placeholder at the moment, it needs more work to handle the cases better.

In the end we might have to add the Android types to the locales definitions (I'd like to avoid that if possible).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/mlocati/cldr-to-gettext-plural-rules has an implementation along the lines of what would be needed. The generated output at https://mlocati.github.io/cldr-to-gettext-plural-rules/ shows the resulting mappings.

@ocean90
Copy link
Member

ocean90 commented Feb 21, 2017

Can we move the plural stuff into its own PR?

@toolstack
Copy link
Contributor Author

@ocean90 Yes, I was considering it after it became obvious it was going to be more complex than at first sight.

@toolstack toolstack force-pushed the 628-support-xliff-in-android-files branch from de1aab4 to e2924d0 Compare February 21, 2017 16:36
@toolstack toolstack merged commit d7eeeed into develop Mar 21, 2017
@toolstack toolstack deleted the 628-support-xliff-in-android-files branch March 21, 2017 17:36
@toolstack toolstack modified the milestones: 3.0, 2.4 Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants